Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove info from ExecutionParams and add operationType #3166

Merged
merged 12 commits into from
Jul 12, 2021
Merged

Remove info from ExecutionParams and add operationType #3166

merged 12 commits into from
Jul 12, 2021

Conversation

ardatan
Copy link
Owner

@ardatan ardatan commented Jul 8, 2021 •
edited
Loading

BREAKING CHANGES;

  • Rename Request to ExecutionRequest (Request is also part of DOM types so this prevents the possible confusion)
  • Drop unnecessary GraphQLResolveInfo in ExecutionRequest
  • Add operationType: OperationTypeNode as a required field in ExecutionRequest
  • Add context in createRequest and createRequestInfo instead of delegateToSchema

It doesn't rely on info.operation.operationType to allow the user to call an operation from different root type.
And it doesn't call getOperationAST again and again to get operation type from the document/operation because we have it in Request and ExecutionParams
https://github.com/ardatan/graphql-tools/pull/3166/files#diff-d4824895ea613dcc1f710c3ac82e952fe0ca12391b671f70d9f2d90d5656fdceR38

Improvements;

  • Memoize defaultExecutor for a single GraphQLSchema so allow getBatchingExecutor to memoize batchingExecutor correctly.
  • And there is no different defaultExecutor is created for subscription and other operation types. Only one executor is used.

Batch executor is memoized by executor reference but createDefaultExecutor didn't memoize the default executor so this memoization wasn't working correctly on batch-execute side.
https://github.com/ardatan/graphql-tools/blob/remove-info-executor/packages/batch-execute/src/getBatchingExecutor.ts#L9

@ardatan ardatan requested a review from yaacovCR July 8, 2021 04:07
@changeset-bot
Copy link

changeset-bot bot commented Jul 8, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 5a8093d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 25 packages
Name Type
@graphql-tools/batch-execute Major
@graphql-tools/delegate Major
@graphql-tools/links Major
@graphql-tools/utils Major
@graphql-tools/wrap Major
@graphql-tools/batch-delegate Patch
@graphql-tools/stitch Patch
@graphql-tools/stitching-directives Patch
@graphql-tools/url-loader Patch
@graphql-tools/graphql-tag-pluck Patch
@graphql-tools/load Patch
@graphql-tools/merge Patch
@graphql-tools/mock Patch
@graphql-tools/node-require Patch
@graphql-tools/relay-operation-optimizer Patch
@graphql-tools/resolvers-composition Patch
@graphql-tools/schema Patch
@graphql-tools/apollo-engine-loader Patch
@graphql-tools/code-file-loader Patch
@graphql-tools/git-loader Patch
@graphql-tools/github-loader Patch
@graphql-tools/graphql-file-loader Patch
@graphql-tools/json-file-loader Patch
@graphql-tools/module-loader Patch
@graphql-tools/prisma-loader Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@theguild-bot
Copy link
Collaborator

theguild-bot commented Jul 8, 2021 •
edited
Loading

The latest changes of this PR are available as alpha in npm (based on the declared changesets):

@graphql-tools/batch-delegate@8.0.0-alpha-c42e811d.0
@graphql-tools/batch-execute@8.0.0-alpha-c42e811d.0
@graphql-tools/delegate@8.0.0-alpha-c42e811d.0
@graphql-tools/graphql-tag-pluck@7.0.0-alpha-c42e811d.0
graphql-tools@8.0.0-alpha-c42e811d.0
@graphql-tools/jest-transform@1.0.0-alpha-c42e811d.0
@graphql-tools/links@8.0.0-alpha-c42e811d.0
@graphql-tools/load@7.0.0-alpha-c42e811d.0
@graphql-tools/apollo-engine-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/code-file-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/git-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/github-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/graphql-file-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/json-file-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/module-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/prisma-loader@6.3.1-alpha-c42e811d.0
@graphql-tools/url-loader@7.0.0-alpha-c42e811d.0
@graphql-tools/merge@6.2.15-alpha-c42e811d.0
@graphql-tools/mock@8.1.4-alpha-c42e811d.0
@graphql-tools/node-require@6.2.5-alpha-c42e811d.0
@graphql-tools/relay-operation-optimizer@6.3.1-alpha-c42e811d.0
@graphql-tools/resolvers-composition@6.3.0-alpha-c42e811d.0
@graphql-tools/schema@8.0.0-alpha-c42e811d.0
@graphql-tools/stitch@8.0.0-alpha-c42e811d.0
@graphql-tools/stitching-directives@2.0.0-alpha-c42e811d.0
@graphql-tools/utils@8.0.0-alpha-c42e811d.0
@graphql-tools/webpack-loader@6.5.0-alpha-c42e811d.0
@graphql-tools/wrap@8.0.0-alpha-c42e811d.0

@yaacovCR
Copy link
Collaborator

See #3180 (comment)

I guess I think it’s ok as is for now

do you feel strongly about this?

dotansimha
dotansimha reviewed Jul 12, 2021
View reviewed changes
Copy link
Collaborator

@dotansimha dotansimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ardatan what's the background for this change?

@dotansimha dotansimha self-requested a review July 12, 2021 15:30
@ardatan
Copy link
Owner Author

ardatan commented Jul 12, 2021 •
edited
Loading

@yaacovCR Updated this PR. If you want to discuss the changes individually, I can split this into smaller PRs.

yaacovCR
yaacovCR reviewed Jul 12, 2021
View reviewed changes
Copy link
Collaborator

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to allow operationType to be optional and inferred from document (rather than info!) if absent.
I think it may in theory be still useful as well when stitching to allow info to be passed to executor just like context.
Other than those points, love it.

packages/delegate/src/index.ts Outdated Show resolved Hide resolved
@ardatan ardatan merged commit c42e811 into master Jul 12, 2021
@ardatan ardatan deleted the remove-info-executor branch July 12, 2021 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaacovCR yaacovCR yaacovCR left review comments

@dotansimha dotansimha dotansimha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ardatan @theguild-bot @yaacovCR @dotansimha